-
Notifications
You must be signed in to change notification settings - Fork 475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add CI job to check coding style #662
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some minor suggestions but looks good overall.
.circleci/config.yml
Outdated
orbs: | ||
shellcheck: circleci/[email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit offtopic: I wonder if we could create custom orbs to make the CI config in the main repo more modular...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely! If there are many repeated tasks there that could also be reused by other repos in the organization, it would be beneficial to split them in orbs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some common stuff. For example the gitter notification command may be a good candidate for an orb (while at it, we could also make it work on Windows). The t_ems_ext
is used by external tests and their definitions are pretty repetitive too. And there are some common steps/templates. Also generally splitting the whole file into smaller bits by grouping similar jobs (build/test jobs, bytecode compare, external tests, etc.) would be great if possible.
We have a whole issue about that (ethereum/solidity#11846) but there are limits to what we can within a single file do and so far I did not consider creating orbs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks, I can take a look on that :)
I can start with the gitter notification then, since there is already an issue for the failing solc-bin nightly builds.
.circleci/config.yml
Outdated
.git/ | ||
node_modules/ | ||
dist/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe shellcheck does need the ./
bit after all? I see in the CI output that it did not skip files inside node_modules:
./node_modules/tape/example/static/build.sh
./node_modules/foreground-child/changelog.sh
No ShellCheck Errors Found
Unless it just prints them without checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, the orb is nice. I see that it for example automatically adds shellcheck.log
as an artifact (if non-empty). That's handy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh no! Indeed it is not skipping those directories. There is an example of the config in the official repo: https://github.com/CircleCI-Public/shellcheck-orb/blob/73e2427de6b0a5d0c27527b608ee62880e3dbfb0/.circleci/config.yml#L22 and the ./
is used. I will put it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum...still it shows. So yeah, it seems that it parses anyway but probably don't check as you mentioned. So let's go without it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try it out locally on a file where you intentionally make a mistake to make sure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It seems that it indeed considers the "ignored" folders when they terminate with /
(e.g. node_modules/
). If I remove the forward slash in the end, it works.
With node_modules/
or ./node_modules/
:
./scripts/wrong.sh
./node_modules/foreground-child/changelog.sh
./node_modules/tape/example/static/build.sh
ShellCheck Errors Found
In ./scripts/wrong.sh line 5:
if [ $1 -eq "1"]; then
^-- SC1009: The mentioned syntax error was in this if expression.
^-- SC1073: Couldn't parse this test expression. Fix to allow more checks.
^-- SC1020: You need a space before the ].
^-- SC1072: Missing space before ]. Fix any mentioned problems and try again.
Without the forward slash in the end ./node_modules
:
./scripts/wrong.sh
ShellCheck Errors Found
In ./scripts/wrong.sh line 5:
if [ $1 -eq "1"]; then
^-- SC1009: The mentioned syntax error was in this if expression.
^-- SC1073: Couldn't parse this test expression. Fix to allow more checks.
^-- SC1020: You need a space before the ].
^-- SC1072: Missing space before ]. Fix any mentioned problems and try again
In conclusion, we need the current directory prefix, and we should not add the end forward slash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. Let's revert to the way it was originally.
Too bad that it's that picky. For a shell these would be perfectly fine and equivalent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I reverted it already. I opened an issue in the orb repo anyway, asking about this behavior.
040952a
to
3671b8c
Compare
So I guess we're done here? Looks ready to merge to me. |
Oh, please squash the commits a bit though. Looks like 3 of the 4 commits are just review tweaks to the first one so they should be squashed into it. |
3ffe1eb
to
86749f5
Compare
This PR adds a CI job to perform coding style checks as suggested here: #659 (comment)
It performs the
eslint
checks for.js
and.ts
files using the npm lint script and checks the.sh
files using the shellcheck circleCI orb.